-
Notifications
You must be signed in to change notification settings - Fork 5.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
stats: fix the index cm sketch for fast analyze #10839
Conversation
Codecov Report
@@ Coverage Diff @@
## master #10839 +/- ##
================================================
- Coverage 81.0684% 80.9174% -0.1511%
================================================
Files 419 419
Lines 88894 88793 -101
================================================
- Hits 72065 71849 -216
- Misses 11600 11709 +109
- Partials 5229 5235 +6 |
return sorted[i] > sorted[j] | ||
}) | ||
numTop = mathutil.MinUint32(uint32(len(counter)), numTop) | ||
lastTopCnt := sorted[numTop-1] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if numTop
is 0, this will panic? executor/analyze.go #334 set numTop
is 0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, they don't have top n and won't go into this function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/run-all-tests |
1 similar comment
/run-all-tests |
/run-unit-test |
2 similar comments
/run-unit-test |
/run-unit-test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
What problem does this PR solve?
The index CM sketch of fast analyze is wrong, it does not consider the prefix columns.
What is changed and how it works?
Built cm sketch for each prefix columns of index, and merge them into one as the final result.
Check List
Tests
Code changes
Side effects
Related changes